-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update functional test expected output #5349
Conversation
Pull Request Test Coverage Report for Build 1499777334
π - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving the first part, thank you ! :) Let's add the end line / end column change on top of it.
pylint/testutils/output_line.py
Outdated
@@ -106,6 +107,11 @@ def from_csv(cls, row: Union[Sequence[str], str]) -> "OutputLine": | |||
if isinstance(row, Sequence): | |||
column = cls._get_column(row[2]) | |||
if len(row) == 5: | |||
warnings.warn( | |||
"In pylint 3.0 tests output should always include the expected confidence" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
π
Used the new updater! Should be better now! |
Used the old updater! Shouldn't fail now π |
@Pierre-Sassoulas @cdce8p I'm working on updating It makes sense to have Adding TLDR: Is |
It's semi public (as maybe used by plugin developer for their tests ?), but as we provide a quick way to upgrade the expected output let's consider that this is a "breaking change" we can do in a minor. |
Yeah that was what I was wondering. However, looking at the code I don't think they should every instantiate an
Fair enough! |
Latest commit is WIP and blocked by #5343. |
I have added additional fixes to the functional test Besides the updating of our own expected output files this PR now includes:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This look amazing. Checking that the column match the expected would take month. I don't think we have a choice but to release as is and wait for issues to get opened.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few notes:
- What is the value in adding
UNDEFINED
basically everywhere? Do we even use the confidence level somewhere? -
Checking that the column match the expected would take month. I don't think we have a choice but to release as is and wait for issues to get opened.
It should be much easier once vscode-python
supports error ranges. Then it's basically just opening the test file and checking that the ranges make sense.
@staticmethod | ||
def _get_py38_none_value(value: T) -> Optional[T]: | ||
"""Handle attributes that are always None on pylint < 3.8 similar to _get_column.""" | ||
if not PY38_PLUS: | ||
# We check the value only for the new better ast parser introduced in python 3.8 | ||
return None # pragma: no cover | ||
return value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That will likely not work for nodes that added end_lineno
and end_col_offset
later. E.g. keyword
, Slice
(both only in 3.9
). Maybe only compare the values if end_lineno is not None
or better a new field in [testoptions]
to overwrite PY38_PLUS
. I.e. if field is specified, ignore end_lineno
check for all versions prior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we ever create messages on those nodes though? Even invalid-slice-index
does not pass an nodes.Slice
as its node
parameter. Could it be that we're fine if we ignore these?
I'm not sure how the testoptions
stuff work so I'll need some guidance if want to add a new one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think only compare the values if end_lineno is not None
is the simpler because I don't know what we could put in the test_option. A condition that needs to succeed or we return None ? Seems like we'll have to launch an eval for that (with all the implication for job launched from fork via github actions) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked into it a bit more and I think min_pyver_end_line
could work? If we convert it to a boolean and pass it to _get_py38_none_value
that should make it future proof while testing on versions that it should.
only compare the values if end_lineno is not None
is what was previously done for confidence
with the __eq__
, I don't think that is the way to go as it allows to just never check it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
min_pyver_end_line
Ho, of course. I did not think of that. Very nice solution ! Should we add one option removing the check for line/end_line/column/end_column (min_pyver_position
to check all 4 values properly only in the version that is the most accurate) or should we add 4 options (min_pyver_end_line
, min_pyver_end_column
... in order to check line/column if line_end or column_end do not exists in other version) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cdce8p Knows more about this than I do so I'll let him respond to this but I believe 3.6
has line
and col_offset
for all nodes that should have it. 3.8
adds end_line
and end_col_offset
for most and 3.9
adds it for some that were missing.
Thus I think we can create one min_pyver_end_position
(?) which defaults to 3.8.0
and can be increased if needed.
Btw, I'm going to be quite busy tonight and tomorrow probably so if we want to release 2.12
today/tomorrow somebody else would likely need to start working on this. Sorry about that!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't worry π min_pyver_end_position
convinced me, do you agree @cdce8p ? By the way it's probably not strictly necessary to release 2.12.0 as we don't have trouble with it right now. We're probably never emitting message for Slice/Keyword nodes ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found some time, see #5386
It allows us to remove the weird |
π although checking the whole functional test directory might still take a considerable amount of time. The visual result in vscode is motivating though. |
963b16d
to
9ba6921
Compare
* Fix checking of ``confidence`` in the unittests Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
9ba6921
to
867ba79
Compare
@DanielNoord should we close #5336 when merging this or should we move #5336 to 2.13 ? I'm planning on finally releasing today :) |
We shouldn't close. |
@Pierre-Sassoulas #5349 (comment) This was not resolved and might become problematic in the future. I think we shouldn't release this before fixing this! |
Ha, right. I was just about to release, thank you for bringing this up π |
Type of Changes
Description
This adds the
confidence
to all expected outputs of functional tests.